[ISSUE #65]Add RocketMQ-on-DLeger group mode deploy by haifzhu · Pull Request #87 · apache/rocketmq-operator

haifzhu · 2021-09-26T01:56:16Z

The Commit is added to add RocketMQ-on-DLeger group mode deploy.

zongtanghu · 2022-10-10T06:20:59Z

I see there are some codes conflicts in this pr, can you resolve this pr in your local enviroments?

RockteMQ-AI · 2026-07-30T10:40:15Z

⚠️ Merge conflict detected

This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts:

git fetch origin
git checkout master
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.

Automated notification by github-manager-bot

RockteMQ-AI

Summary

This PR modifies 16 file(s) with 2427 lines of diff. No test changes detected — consider adding test coverage.

Automated review by github-manager-bot

Additional notes (not anchored to a changed line)

[INFO] README.md:1 — Large diff (2427 lines). Consider breaking into smaller, focused PRs for easier review. (line outside diff)

RockteMQ-AI · 2026-08-05T14:55:59Z

@@ -34,14 +34,18 @@ type BrokerSpec struct {
 	Size int `json:"size"`


No test changes detected alongside source modifications. Consider adding tests to cover the changes.

RockteMQ-AI

Summary

PR received and logged for review. This PR requires detailed code review by a maintainer.

Diff size: 2427 lines
Author: haifzhu (NONE)

Automated review by RockteMQ-AI

RockteMQ-AI

Summary

Review of PR #87: [ISSUE #65]Add RocketMQ-on-DLeger group mode deploy

Findings: 4 issue(s) identified (2 critical).
CLA: unknown

Please address the inline comments above.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-23T19:58:41Z

+            - brokerImage
+            - imagePullPolicy
+            - nameServers
+            - allowRestart


Adding enableDLeger to the required list is a breaking API change for the served v1alpha1 Broker CRD. Existing Broker CRs that do not specify this field will fail validation on updates, and the schema does not declare a default. The field should be optional (removed from required) to preserve backward compatibility.

RockteMQ-AI · 2026-08-23T19:58:41Z

  size: 1
  # nameServers is the [ip:port] list of name service
  nameServers: ""
+  # Whether enable rocketmq-on-dleger group deploy


README documents enableDLeger: false as optional with an implicit default, but the CRD schema marks the field as required and does not declare a default value. The documented behavior does not match the schema.

RockteMQ-AI · 2026-08-23T19:58:41Z

            nameServers:
              description: NameServers defines the name service list e.g. 192.168.1.1:9876;192.168.1.2:9876
              type: string
+            enableDLeger:


The diff only updates the CRD schema and README; no controller, StatefulSet generation, or broker configuration code is modified to consume the new enableDLeger field. As submitted, the field has no effect and the RocketMQ-on-DLeger deployment mode is not implemented.

RockteMQ-AI · 2026-08-23T19:58:41Z

+            - brokerImage
+            - imagePullPolicy
+            - nameServers
+            - allowRestart


No controller tests, reconciliation tests, or RBAC changes are included for the new DLeger mode, affinity, or tolerations support. A feature affecting broker topology and scheduling should have test coverage and any required RBAC updates.

RockteMQ-AI

Summary

The PR introduces DLedger group mode with significant structural changes, but has critical blocking issues: an infinite polling loop in DLegerHostIp that can deadlock the controller, a data race on a shared global variable, and a panic when NameService size is 0. Additionally, marking enableDLeger as required in the CRD is a backward-incompatible breaking change.

Findings

[CRITICAL] pkg/controller/broker/broker_controller.go:131 — DLegerHostIp contains an unbounded for {} loop with time.Sleep that polls forever until the Service exists. This blocks the reconcile goroutine indefinitely if the Service creation failed (e.g., RBAC denial, quota exceeded). The controller will deadlock — no other reconcile events can be processed on this worker. Replace with a bounded retry or, better, return the error from Reconcile and let the controller framework requeue with backoff.
[WARNING] pkg/controller/broker/broker_controller.go:108 — CreateService logs the error from r.client.Create but does not return it. The caller proceeds to build a StatefulSet and call DLegerHostIp which will then block forever waiting for the Service that failed to create. This should return an error to the caller so Reconcile can requeue.
[WARNING] pkg/controller/broker/broker_controller.go:196 — DLeger peer construction and CreateService/DLegerHostIp calls are unconditional — they run even when broker.Spec.EnableDLeger is false. This creates unnecessary Services and blocks on them for every non-DLedger deployment. Wrap the DLeger-specific logic in an if broker.Spec.EnableDLeger guard.
[CRITICAL] pkg/controller/nameservice/nameservice_controller.go:161 — getNameServiceName panics when nameService.Spec.Size is 0: the loop body never executes, nameserviceName remains "", and nameserviceName[:len(nameserviceName)-1] evaluates to ""[:-1] which is an out-of-bounds slice. Add a guard for Size == 0.
[WARNING] pkg/share/share.go:28 — NameServersServiceStr is a package-level mutable global written by the NameService controller (nameservice_controller.go:168) and read by the Broker controller (broker_controller.go:575). This is a data race in a concurrent controller-runtime environment where both reconcilers run in parallel goroutines. Use a thread-safe mechanism (mutex, or pass via CR status) instead of a bare global string.
[WARNING] deploy/crds/rocketmq_v1alpha1_broker_crd.yaml:843 — enableDLeger is added to the required fields list. This is a breaking change — any existing Broker CR that does not specify enableDLeger will be rejected by the API server on update. New optional fields should not be required; rely on the Go zero-value (false) or add a default via admission webhook.
[WARNING] pkg/apis/rocketmq/v1alpha1/broker_types.go:46 — Affinity is declared as corev1.Affinity (value type) without omitempty. An empty Affinity struct will always be serialized into the StatefulSet pod spec even when the user sets none, and the CRD makes it required. Use *corev1.Affinity (pointer) with json:"affinity,omitempty" so it is nil when unset.
[WARNING] pkg/apis/rocketmq/v1alpha1/broker_types.go:47 — Tolerations and other new fields (Env in NameService) also lack omitempty in their JSON tags. This forces users to provide empty arrays in their CRs and makes the CRD schema unnecessarily strict.
[WARNING] pkg/controller/console/console_controller.go:198 — The hardcoded JAVA_OPTS env var (containing -Drocketmq.namesrv.addr=...) was removed without a replacement mechanism. Users who relied on the operator injecting the name server address into the console will now get a console that cannot connect to RocketMQ. The example CRs add JAVA_OPTS manually, but existing deployments that upgrade will silently break. Consider keeping a default or documenting this as a breaking change.
[INFO] pkg/controller/broker/broker_controller.go:137 — DLegerHostIp returns svcObj.Spec.ClusterIP from a Service that may have just been created. For non-headless Services (the broker services here have no ClusterIP: "None"), the ClusterIP is assigned asynchronously by the API server. A tight poll loop may read an empty ClusterIP. Consider using a headless Service + pod DNS (e.g., <pod>.<svc>.<ns>.svc.cluster.local) for stable addressing.
[INFO] pkg/controller/nameservice/nameservice_controller.go:344 — The comment says // Set bind for broker crd and svc but this is in the NameService controller and the owner reference is set to nameService, not a broker. Copy-paste artifact — update the comment to reflect the actual owner.
[INFO] images/broker/alpine/brokerGenConfig.sh:34 — The shell test [ ! -z $ENABLE_DLEGER ] is unquoted. If ENABLE_DLEGER is unset (not just empty), this becomes [ ! -z ] which evaluates to true (tests the -z flag itself as a non-empty string), enabling DLedger unintentionally. Quote it: [ ! -z "$ENABLE_DLEGER" ] or use [ -n "$ENABLE_DLEGER" ].
[WARNING] pkg/controller/broker/broker_controller.go:241 — In the scale-down/update path, CreateService is called for replicas inside the nameServers-update loop but never for the master (replicaIndex 0). If the master's Service was deleted externally, it will not be recreated during updates, while replica services will be. This is inconsistent with the initial creation path at line 196.
[INFO] pkg/controller/broker/broker_controller.go:571 — share.NameServersStr was replaced with share.NameServersServiceStr in the broker env. NameServersServiceStr is built from the CR name and assumes a specific headless service naming convention. If the NameService controller hasn't reconciled yet, this value may be stale or empty, causing brokers to start with no valid name server address.
[INFO] pkg/share/share.go:28 — The comment on NameServersServiceStr says // NameServersStr is the name server list — it's a copy of the comment above it. Update to describe what this variable actually holds (service-based name server addresses).

Automated review by github-manager-bot

RockteMQ-AI · 2026-09-01T08:53:19Z

+	reqLogger.Info("Get DLeger Host IP lists...")
+
+	statefulSetName := getBrokerStatefulSetName(broker, brokerGroupIndex, replicaIndex)
+


DLegerHostIp contains an unbounded for {} loop with time.Sleep that polls forever until the Service exists. This blocks the reconcile goroutine indefinitely if the Service creation failed (e.g., RBAC denial, quota exceeded). The controller will deadlock — no other reconcile events can be processed on this worker. Replace with a bounded retry or, better, return the error from Reconcile and let the controller framework requeue with backoff.

RockteMQ-AI · 2026-09-01T08:53:19Z

 	scheme *runtime.Scheme
 }

+func (r *ReconcileBroker) CreateService(request reconcile.Request, broker *rocketmqv1alpha1.Broker, brokerGroupIndex int, replicaIndex int) {


CreateService logs the error from r.client.Create but does not return it. The caller proceeds to build a StatefulSet and call DLegerHostIp which will then block forever waiting for the Service that failed to create. This should return an error to the caller so Reconcile can requeue.

RockteMQ-AI · 2026-09-01T08:53:19Z

 	replicaPerGroup := broker.Spec.ReplicaPerGroup
 	reqLogger.Info("brokerGroupNum=" + strconv.Itoa(share.GroupNum) + ", replicaPerGroup=" + strconv.Itoa(replicaPerGroup))
+
 	for brokerGroupIndex := 0; brokerGroupIndex < share.GroupNum; brokerGroupIndex++ {


DLeger peer construction and CreateService/DLegerHostIp calls are unconditional — they run even when broker.Spec.EnableDLeger is false. This creates unnecessary Services and blocks on them for every non-DLedger deployment. Wrap the DLeger-specific logic in an if broker.Spec.EnableDLeger guard.

RockteMQ-AI · 2026-09-01T08:53:19Z

 	return r.updateNameServiceStatus(instance, request, true)
 }

+func getNameServiceName(nameService *rocketmqv1alpha1.NameService) string {


getNameServiceName panics when nameService.Spec.Size is 0: the loop body never executes, nameserviceName remains "", and nameserviceName[:len(nameserviceName)-1] evaluates to ""[:-1] which is an out-of-bounds slice. Add a guard for Size == 0.

RockteMQ-AI · 2026-09-01T08:53:19Z

 	// NameServersStr is the name server list
 	NameServersStr = ""

+	// NameServersStr is the name server list


NameServersServiceStr is a package-level mutable global written by the NameService controller (nameservice_controller.go:168) and read by the Broker controller (broker_controller.go:575). This is a data race in a concurrent controller-runtime environment where both reconcilers run in parallel goroutines. Use a thread-safe mechanism (mutex, or pass via CR status) instead of a bare global string.

RockteMQ-AI · 2026-09-01T08:53:20Z

+func (r *ReconcileNameService) CreateService(request reconcile.Request, nameService *rocketmqv1alpha1.NameService) {
+	reqLogger := log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)
+	reqLogger.Info("Create a Name Service...")
+


The comment says // Set bind for broker crd and svc but this is in the NameService controller and the owner reference is set to nameService, not a broker. Copy-paste artifact — update the comment to reflect the actual owner.

RockteMQ-AI · 2026-09-01T08:53:20Z

        sed -i 's/brokerRole=.*/brokerRole=SLAVE/g' $BROKER_CONFIG_FILE
    fi
+
+    # Enable RocketMQ-on-DLedger Group


The shell test [ ! -z $ENABLE_DLEGER ] is unquoted. If ENABLE_DLEGER is unset (not just empty), this becomes [ ! -z ] which evaluates to true (tests the -z flag itself as a non-empty string), enabling DLedger unintentionally. Quote it: [ ! -z "$ENABLE_DLEGER" ] or use [ -n "$ENABLE_DLEGER" ].

RockteMQ-AI · 2026-09-01T08:53:20Z

@@ -193,8 +241,14 @@ func (r *ReconcileBroker) Reconcile(request reconcile.Request) (reconcile.Result
 			for brokerGroupIndex := 0; brokerGroupIndex < broker.Spec.Size; brokerGroupIndex++ {


In the scale-down/update path, CreateService is called for replicas inside the nameServers-update loop but never for the master (replicaIndex 0). If the master's Service was deleted externally, it will not be recreated during updates, while replica services will be. This is inconsistent with the initial creation path at line 196.

RockteMQ-AI · 2026-09-01T08:53:20Z

@@ -453,10 +569,10 @@ func (r *ReconcileBroker) getBrokerStatefulSet(broker *rocketmqv1alpha1.Broker,

 }



share.NameServersStr was replaced with share.NameServersServiceStr in the broker env. NameServersServiceStr is built from the CR name and assumes a specific headless service naming convention. If the NameService controller hasn't reconciled yet, this value may be stale or empty, causing brokers to start with no valid name server address.

RockteMQ-AI · 2026-09-01T08:53:20Z

 	// NameServersStr is the name server list
 	NameServersStr = ""

+	// NameServersStr is the name server list


The comment on NameServersServiceStr says // NameServersStr is the name server list — it's a copy of the comment above it. Update to describe what this variable actually holds (service-based name server addresses).

haifzhu added 5 commits September 26, 2021 09:45

Add RocketMQ-on-DLeger group mode deploy

915821a

Add RocketMQ-on-DLeger group mode deploy

90eac73

add affinity and tolerations

428c3b4

修改crds

b192258

add env to nameserver

6575669

caigy changed the title ~~Add RocketMQ-on-DLeger group mode deploy~~ [ISSUE #65]Add RocketMQ-on-DLeger group mode deploy Jul 13, 2022

caigy added the enhancement New feature or request label Jul 13, 2022

RockteMQ-AI reviewed Aug 5, 2026

View reviewed changes

RockteMQ-AI reviewed Aug 20, 2026

View reviewed changes

RockteMQ-AI suggested changes Aug 23, 2026

View reviewed changes

RockteMQ-AI suggested changes Sep 1, 2026

View reviewed changes

		@@ -34,14 +34,18 @@ type BrokerSpec struct {
		Size int `json:"size"`

		reqLogger.Info("Get DLeger Host IP lists...")

		statefulSetName := getBrokerStatefulSetName(broker, brokerGroupIndex, replicaIndex)

		@@ -193,8 +241,14 @@ func (r *ReconcileBroker) Reconcile(request reconcile.Request) (reconcile.Result
		for brokerGroupIndex := 0; brokerGroupIndex < broker.Spec.Size; brokerGroupIndex++ {

		@@ -453,10 +569,10 @@ func (r ReconcileBroker) getBrokerStatefulSet(broker rocketmqv1alpha1.Broker,

		}

Uh oh!

Conversation

haifzhu commented Sep 26, 2021

Uh oh!

zongtanghu commented Oct 10, 2022

Uh oh!

RockteMQ-AI commented Jul 30, 2026

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Additional notes (not anchored to a changed line)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants